Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

clippy: Removes redundant async blocks #31526

Merged
merged 1 commit into from
May 9, 2023
Merged

clippy: Removes redundant async blocks #31526

merged 1 commit into from
May 9, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented May 6, 2023 •
edited
Loading

Problem

Clippy throws warnings on the next nightly version (1.70.0), which blocks upgrading our nightly Rust (see #31381).

Here's an example:

warning: this async expression only awaits a single future
    --> rpc/src/rpc.rs:3522:22
     |
3522 |             Box::pin(async move { meta.get_signature_statuses(signatures, config).await })
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `meta.get_signature_statuses(signatures, config)`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_async_block
     = note: `#[warn(clippy::redundant_async_block)]` on by default

Summary of Changes

Remove redundant async away blocks

@brooksprumo brooksprumo self-assigned this May 6, 2023
@brooksprumo brooksprumo mentioned this pull request May 6, 2023
Merged
@brooksprumo brooksprumo marked this pull request as ready for review May 6, 2023 15:47
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #31526 (61f3fe2) into master (775639c) will decrease coverage by 0.1%.
The diff coverage is 85.7%.

@@            Coverage Diff            @@
##           master   #31526     +/-   ##
=========================================
- Coverage    81.4%    81.3%   -0.1%     
=========================================
  Files         731      731             
  Lines      205088   205084      -4     
=========================================
- Hits       166944   166914     -30     
- Misses      38144    38170     +26

@CriesofCarrots
Copy link
Contributor

Out of curiosity, why is the example in the description not one of the instances fixed in this PR?

@brooksprumo
Copy link
Contributor Author

Out of curiosity, why is the example in the description not one of the instances fixed in this PR?

Heh, good eye! Here's the output from running clippy --fix:

warning: `solana-rpc` (lib) generated 13 warnings (run `cargo clippy --fix --lib -p solana-rpc` to apply 13 suggestions)
warning: failed to automatically apply fixes suggested by rustc to crate `solana_rpc`

after fixes were automatically applied the compiler reported errors within these files:

  * rpc/src/rpc.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see 
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

There were a few files like this too. So looks like rpc.rs didn't end up modified (on disk) due to the bug and thus not added to this commit. I used that one as the example in the PR description since it was at the bottom of the output and one of the first I saw.

Do you think I should manually add the changes from rpc.rs, or better to handle them separately?

brooksprumo
brooksprumo commented May 7, 2023
View reviewed changes
quic-client/src/quic_client.rs
Comment on lines 161 to +164
let _lock = ASYNC_TASK_SEMAPHORE.acquire();
let inner = self.inner.clone();

let _handle = RUNTIME.spawn(async move { send_data_async(inner, data).await });
let _handle = RUNTIME.spawn(send_data_async(inner, data));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change affect behavior? I see we grab an async semaphore at the top of the function; does not running the send_data_async in an async block break anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it does not

async fn send_data_async(
    connection: Arc<NonblockingQuicConnection>,
    buffer: Vec<u8>,
) -> TransportResult<()> {

Since send_data_async is an async fn, when you call it what happens is that it builds its return future and returns it. It only builds it tho, it does not poll it. Futures don't make progress until they're polled, so effectively the future will start executing only inside the task it's spawned on.

It would be different if send_data_async built the future manually, eg:

fn send_data_async(
    connection: Arc<NonblockingQuicConnection>,
    buffer: Vec<u8>,
) -> SendDataFuture {
    do_something_before_creating_the_future();
    SendDataFuture {
       ...
    }
}

In this case do_something_before_creating_the_future does execute before the resulting future is polled. So before landing this we need to double check if we're creating any manual futures, and if we're doing actual work (eg locking things) before returning them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

I went through and audited all functions that were inside the async { .await } blocks, and they all were async fn (i.e. not creating any manual futures. So I think that means this PR is ok.

@brooksprumo
Copy link
Contributor Author

I plan on merging this PR tomorrow unless there are any objections.

@CriesofCarrots
Copy link
Contributor

Do you think I should manually add the changes from rpc.rs, or better to handle them separately?

Either is fine with me.

@ilya-bobyr
Copy link
Contributor

I've made a similar fix here: #30808
There were cases that clippy flagged incorrectly.
It seems to be missing implicitly captured values (maybe only some?) in the analysis.
In particular, in rpc.rs.
I had to apply exceptions in a few cases.
In my PR, I could not enable clippy exceptions, as they are only supported starting at 1.69.
But as we are now on 1.69, it should be possible.

I wonder if after applying this PR you actually see all the Clippy warnings about redundant async/await really disappearing.
Just as Tyera pointed out, the warning you used in the PR description is not addressed in the patch. Is it gone after applying this patch?

@brooksprumo
Copy link
Contributor Author

I wonder if after applying this PR you actually see all the Clippy warnings about redundant async/await really disappearing. Just as Tyera pointed out, the warning you used in the PR description is not addressed in the patch. Is it gone after applying this patch?

I would expect the same warnings to still exist in rpc.rs, since they are not addressed in this PR.

@brooksprumo brooksprumo merged commit 6e342de into solana-labs:master May 9, 2023
@brooksprumo brooksprumo deleted the clippy/async branch May 9, 2023 13:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@alessandrod alessandrod alessandrod approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@brooksprumo brooksprumo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brooksprumo @CriesofCarrots @ilya-bobyr @alessandrod